CI: https://ci.nodejs.org/job/node-test-pull-request/2510/

The test in question failed during the CI run.

It looks like the output in Windows is different:

# assert.js:90
#   throw new assert.AssertionError({
#   ^
# AssertionError: '(node:4660) Target process: 655555 doesn\'t exist.' === '(node:4660) There was an internal error in Node\'s debugger. Please report this bug.'
#     at ChildProcess.<anonymous> (C:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2013\label\win2008r2\test\parallel\test-debugger-pid.js:28:10)
#     at emitOne (events.js:96:13)
#     at ChildProcess.emit (events.js:188:7)
#     at C:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2013\label\win2008r2\test\parallel\test-debugger-pid.js:18:16
#     at Array.forEach (native)
#     at Socket.onData (C:\workspace\node-test-binary-windows\RUN_SUBSET\3\VS_VERSION\vs2013\label\win2008r2\test\parallel\test-debugger-pid.js:17:8)
#     at emitOne (events.js:96:13)
#     at Socket.emit (events.js:188:7)
#     at readableAddChunk (_stream_readable.js:172:18)
#     at Socket.Readable.push (_stream_readable.js:130:10)

Does anyone know if it has been always like this?

@santigimeno Grep for the Windows version of DebugProcess() in src/node.cc.

That error you're seeing is the 'uncaughtException' event handler in lib/_debugger.js. I speculate this code fails with the error message from OpenProcess() call in DebugProcess() instead of the expected ESRCH exception, and that ends up triggering the handler.

@bnoordhuis yes, thanks for looking into it. I can confirm your speculation and the error reported in Windows is:

# c:\workspace\node-stress-single-test\nodes\win2012r2\Release\node.exe debug -p 655555
# debug> (node:1776) There was an internal error in Node's debugger. Please report this bug.
# The parameter is incorrect.
# Error: The parameter is incorrect.
#     at Error (native)
#     at Interface.trySpawn (_debugger.js:1668:17)
#     at Interface.run (_debugger.js:1043:10)
#     at Timeout._onTimeout (_debugger.js:835:10)
#     at tryOnTimeout (timers.js:224:11)
#     at Timer.listOnTimeout (timers.js:198:5)

I have updated the PR so it handles the Windows case accordingly.

CI run: https://ci.nodejs.org/job/node-test-commit/3248/. Apart from the usual suspects the run looks good.

I think I'd rather just skip the test on Windows instead of checking for that 'There was an internal error' message.

I'm also checking for the The parameter is incorrect string that seemed a little more specific. Anyway, I'll skip it if you prefer it that way.

Some test coverage is better than no coverage, I suppose. I'll leave it up to you, LGTM either way.

/cc @nodejs/testing what do you think? It's fine by me either way

LGTM the way it is. Fine skipping on Windows too. Slight preference for leaving it the way it is so that if the behavior ever becomes consistent, we find out rather than forever skipping the test.

One last CI run: https://ci.nodejs.org/job/node-test-pull-request/2573/ and it's green.

Finally I left the Windows part. Thanks for the reviews!

Looks like this was landed in 588cfc5

@Trott lts?

@thealphanerd I think perhaps you meant to ping @santigimeno rather than me?

@thealphanerd No, it should not be backported. It works as expected in 4.x.